Added `d1 export --local` support #6073

geelen · 2024-06-18T06:54:50Z

This uses a new miniflare-only debug pragma PRAGMA miniflare_d1_export to trigger the dump as D1 provides no in-Worker API (which Miniflare uses) to generate exports.

I'm open to other ways to implement this.

What this PR solves / how to test

Fixes limitation with the previous iteration of the export functionality. Previously, all d1 export --local commands would fail.

Author has addressed the following

Tests
- TODO (before merge)
- Included
- Not necessary because:
E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
- I don't know
- Required / Maybe required
- Not required because: covered by local tests
Changeset (Changeset guidelines)
- TODO (before merge)
- Included
- Not necessary because:
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s):
- Not necessary because: documentation doesn't say --local doesn't work, it just emphasises --remote (which still makes sense to be the default).

changeset-bot · 2024-06-18T06:54:54Z

🦋 Changeset detected

Latest commit: 6f94501

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
miniflare	Minor
wrangler	Minor
@cloudflare/pages-shared	Patch
@cloudflare/vitest-pool-workers	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-06-18T07:00:47Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9691836885/npm-package-wrangler-6073

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6073/npm-package-wrangler-6073

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9691836885/npm-package-wrangler-6073 dev path/to/script.js

Additional artifacts:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9691836885/npm-package-create-cloudflare-6073 --no-auto-update

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9691836885/npm-package-cloudflare-kv-asset-handler-6073

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9691836885/npm-package-miniflare-6073

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9691836885/npm-package-cloudflare-pages-shared-6073

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9691836885/npm-package-cloudflare-vitest-pool-workers-6073

Note that these links will no longer work once the GitHub Actions artifact expires.

wrangler@3.62.0 includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20240620.0
`workerd`	1.20240620.1	1.20240620.1
`workerd --version`	1.20240620.1	2024-06-20

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

packages/miniflare/src/workers/d1/dumpSql.ts

justin-mp

For my own education, does this PR mean that PRAGMA miniflare_d1_export(?,?,?); is part of our public API now? I don't think we want it to be so. How do we prevent people from writing that in their own queries?

packages/miniflare/src/workers/d1/dumpSql.ts

justin-mp · 2024-06-25T10:43:47Z

packages/miniflare/src/workers/d1/dumpSql.ts

+		}
+	}
+
+	if (!noSchema) {


Does it matter that this query appears first in shell.c.in?

Does it? This is supposed to match up with https://github.com/sqlite/sqlite/blob/105c20648e1b05839fd0638686b95f2e3998abcb/src/shell.c.in#L8472, i.e. directly after run_schema_dump_query (which dumps schema & data), it checks SHFLG_DumpDataOnly) is not set before exporting all ('index','trigger','view') rows of sqlite_schema

Have I got that wrong?

No, you're correct.

justin-mp · 2024-06-25T10:49:18Z

packages/wrangler/src/__tests__/d1/export.test.ts

-			runWrangler("d1 export db --local --output /tmp/test.sql")
-		).rejects.toThrowError(
-			`Local imports/exports will be coming in a future version of Wrangler.`
+	it("should handle local", async () => {


You should probably add tests for the edge cases like quoted names adding the "IF NOT EXISTS" as well as properly quoting things like keywords.

I have all these inside the D1 codebase, but see #6073 (comment) I think it makes sense for this file to live in a separate package with a whole suite of tests (as well as a test runner that runs native SQLite for comparison like the D1 suite does).

Imo that's a follow-up task. For the moment I'm just keeping the two implementations in sync manually.

I agree it could be a follow-up task.

geelen · 2024-06-27T06:33:23Z

For my own education, does this PR mean that PRAGMA miniflare_d1_export(?,?,?); is part of our public API now? I don't think we want it to be so. How do we prevent people from writing that in their own queries?

It's part of miniflare's implementation of D1, but not present anywhere else. It's not accessible from wrangler d1 execute --local since you can't pass parameters but technically if someone wrote it in a worker and passed in at least two params they'd get a sql dump in local dev (and an error in production).

We could refactor dumpSql in this PR to use the public D1 API rather than the internal DO one (and then wrangler export would just send lots of queries to the D1 rather than a single debug pragma), but that would mean it's much harder to reuse the specific dumpSql code that's present in D1's internal codebase. And so I've erred on the side of making it easier to keep them consistent and accept leaking an implementation detail.

Ideally, Miniflare would make it possible to do an end-run around the D1 shim and hit an object directly with an RPC call, but as far as I can find out that's not possible.

This uses a new miniflare-only debug pragma `PRAGMA miniflare_d1_export` to trigger the dump as D1 provides no in-Worker API (which Miniflare uses) to generate exports.

justin-mp · 2024-06-27T10:51:34Z

packages/miniflare/src/workers/d1/dumpSql.ts

+		}
+	}
+
+	if (!noSchema) {


No, you're correct.

justin-mp · 2024-06-27T10:52:07Z

packages/wrangler/src/__tests__/d1/export.test.ts

-			runWrangler("d1 export db --local --output /tmp/test.sql")
-		).rejects.toThrowError(
-			`Local imports/exports will be coming in a future version of Wrangler.`
+	it("should handle local", async () => {


I agree it could be a follow-up task.

geelen requested review from a team as code owners June 18, 2024 06:54

geelen mentioned this pull request Jun 24, 2024

🐛 BUG: mockOAuthFlow breaks Miniflare in tests #6132

Closed

geelen force-pushed the glen/d1-export-local branch 3 times, most recently from 3025559 to b2ada25 Compare June 24, 2024 23:09

andyjessop reviewed Jun 25, 2024

View reviewed changes

packages/miniflare/src/workers/d1/dumpSql.ts Show resolved Hide resolved

andyjessop reviewed Jun 25, 2024

View reviewed changes

packages/miniflare/src/workers/d1/dumpSql.ts Outdated Show resolved Hide resolved

justin-mp reviewed Jun 25, 2024

View reviewed changes

Added d1 export --local support

6f94501

This uses a new miniflare-only debug pragma `PRAGMA miniflare_d1_export` to trigger the dump as D1 provides no in-Worker API (which Miniflare uses) to generate exports.

geelen force-pushed the glen/d1-export-local branch from b2ada25 to 6f94501 Compare June 27, 2024 06:34

andyjessop approved these changes Jun 27, 2024

View reviewed changes

justin-mp approved these changes Jun 27, 2024

View reviewed changes

geelen merged commit 7ed675e into main Jun 28, 2024
19 checks passed

geelen deleted the glen/d1-export-local branch June 28, 2024 00:01

workers-devprod mentioned this pull request Jun 28, 2024

Version Packages #6153

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added `d1 export --local` support #6073

Added `d1 export --local` support #6073

geelen commented Jun 18, 2024 •

edited

Loading

changeset-bot bot commented Jun 18, 2024 •

edited

Loading

github-actions bot commented Jun 18, 2024 •

edited

Loading

justin-mp left a comment

justin-mp Jun 25, 2024

geelen Jun 27, 2024

justin-mp Jun 27, 2024

justin-mp Jun 25, 2024

geelen Jun 27, 2024

justin-mp Jun 27, 2024

geelen commented Jun 27, 2024

justin-mp Jun 27, 2024

justin-mp Jun 27, 2024

Added d1 export --local support #6073

Added d1 export --local support #6073

Conversation

geelen commented Jun 18, 2024 • edited Loading

What this PR solves / how to test

Author has addressed the following

changeset-bot bot commented Jun 18, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Jun 18, 2024 • edited Loading

justin-mp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geelen commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Added `d1 export --local` support #6073

Added `d1 export --local` support #6073

geelen commented Jun 18, 2024 •

edited

Loading

changeset-bot bot commented Jun 18, 2024 •

edited

Loading

github-actions bot commented Jun 18, 2024 •

edited

Loading